-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add response id to request error. (bunq/sdk_php#88) #93
Add response id to request error. (bunq/sdk_php#88) #93
Conversation
@sandervdo please 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OGKevin all yours. 😄
src/Exception/ExceptionFactory.php
Outdated
{ | ||
private static function generateErrorMessage( | ||
int $responseCode, | ||
array $messages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allMessage
src/Exception/ExceptionFactory.php
Outdated
{ | ||
$errorMessage = static::generateErrorMessage($responseCode, $messages); | ||
public static function createExceptionForResponse( | ||
array $messages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allMessage
src/Exception/ExceptionFactory.php
Outdated
@@ -20,6 +20,8 @@ class ExceptionFactory | |||
* Formatting constants | |||
*/ | |||
const FORMAT_RESPONSE_CODE_LINE = 'HTTP Response Code: %s'; | |||
const FORMAT_RESPONSE_ID = 'The response id to help bunq debug: %s'; | |||
const FORMAT_ERROR_MESSAGE_LINE = 'Error message: %s'; | |||
const GLUE_ERROR_MESSAGES = "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR_MESSAGE_GLUE? ERROR_MESSAGE_SEPARATOR might even be better
* The index of the first item in an array. | ||
*/ | ||
const INDEX_FIRST = 0; | ||
|
||
/** | ||
* @param ResponseInterface $response | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid, as you can see this particular line has not been changed in this pr and therefore only a snippet of the doc block is show. If you look at the original file you see that this doc block is correct 🙃 see:
* @return ResponseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, missed that one. Ignore then. 😄
@@ -65,11 +80,32 @@ private function fetchErrorDescriptions(array $errorArray): array | |||
{ | |||
$errorDescriptions = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allErrorDescription
@@ -49,9 +64,9 @@ private function fetchErrorMessages(ResponseInterface $response): array | |||
$responseBody = $response->getBody(); | |||
$responseBodyInJson = json_decode($responseBody, true); | |||
|
|||
if ($responseBodyInJson !== false){ | |||
if ($responseBodyInJson !== false) { | |||
return $this->fetchErrorDescriptions($responseBodyInJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchAllErrorDescription
tests/Http/ErrorResponseIdTest.php
Outdated
@@ -0,0 +1,29 @@ | |||
<?php | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
tests/Http/ErrorResponseIdTest.php
Outdated
*/ | ||
public function testBadRequestWitResponseId() | ||
{ | ||
$caughtExcretion = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean exception?
tests/Http/ErrorResponseIdTest.php
Outdated
$caughtExcretion = null; | ||
|
||
try { | ||
UserPerson::get(static::$apiContext, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number
@sandervdo all yours again, please 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OGKevin done. 1 more plural I noticed and see comment on the naming of your constant (might be a SDK specific thing) :)
src/Exception/ExceptionFactory.php
Outdated
|
||
return static::glueMessages(array_merge([$lineResponseCode], $messages)); | ||
return static::glueMessages([$lineResponseCode, $lineResponseId, $lineErrorMessage]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh crap, I see a plural here now too. 🙈
src/Exception/ExceptionFactory.php
Outdated
@@ -78,6 +96,6 @@ private static function generateErrorMessage(int $responseCode, array $messages) | |||
*/ | |||
private static function glueMessages(array $messages): string | |||
{ | |||
return implode(self::GLUE_ERROR_MESSAGES, $messages); | |||
return implode(self::SEPARATOR_ERROR_MESSAGES, $messages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would call it ERROR_MESSAGE_SEPARATOR
then, (big -> small)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally group constants together, so in this case this constant belongs to the SEPARATOR_
constant group. As you can have multiple separators and then the constant names will begin with SEPARATOR_XXXXXX
.
@sandervdo all yours again, please 👀 |
@andrederoos All yours. |
@OGKevin branche is out-of-date |
…m a failed request. (#88)
7b09710
to
3653cf3
Compare
@andrederoos my bad, updated 🤦♂️ |
Closes #88